Skip to content

Fix compatibility with PG16 #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Conversation

df7cb
Copy link
Contributor

@df7cb df7cb commented Jul 31, 2023

  • Replace Abs with fabs
  • Remove DatumGetPointer where PG16 got stricter typechecking
  • Annotate expected/moc100 output files with PG major and architecture bits they are meant for
  • Add expected output files for moc100 on PG16

@esabol
Copy link
Contributor

esabol commented Jul 31, 2023

Would it be premature to add PostgreSQL 16 to the Travis CI? Does apt.postgresql.org.sh support PostgreSQL 16 yet? If 16 does work, then I suggest adding it to .travis.yml in this PR.

Just curious, where did Abs() come from in the first place? Is that an older PostgreSQL thing?

@vitcpp
Copy link
Contributor

vitcpp commented Aug 1, 2023

I'm not sure how the reference output file moc100_x.out is chosen for comparison. Is it performed automatically by test scripts?

@df7cb
Copy link
Contributor Author

df7cb commented Aug 1, 2023

I'm not sure how the reference output file moc100_x.out is chosen for comparison. Is it performed automatically by test scripts?

https://www.postgresql.org/docs/current/regress-variant.html

Would it be premature to add PostgreSQL 16 to the Travis CI? Does apt.postgresql.org.sh support PostgreSQL 16 yet? If 16 does work, then I suggest adding it to .travis.yml in this PR.

Of course PG16 is already supported. I'll add that shortly.

Just curious, where did Abs() come from in the first place? Is that an older PostgreSQL thing?

Abs was removed in PG16.

@vitcpp
Copy link
Contributor

vitcpp commented Aug 1, 2023

@df7cb Thank you for the explanation. I'm ok with the patch but we have to fix tests for pg16. I'm not sure how to fix it yet. May be to change snapshot to the latest one?

@df7cb
Copy link
Contributor Author

df7cb commented Aug 1, 2023

The problems that the testsuite has are independent from this pull request.

  • On Ubuntu bionic 18.04 (now), there is no PG16 since bionic's zstd is too old so I don't provide that on apt.postgresql.org
  • On Ubuntu jammy 22.04, all jobs fail because of compiler warnings (-Werror) in other areas of the code
  • On Ubuntu focal 20.04 (which I now put into this PR), only PG10 and PG11 fail, again for something that this PR doesn't touch

The issues should be fixed, but in a separate PR. This PR does fix the actual PG16 problems and I already applied it in the packages on apt.postgresql.org.

@vitcpp
Copy link
Contributor

vitcpp commented Aug 1, 2023

I propose to enable and fix PG16 tests in another PR and then apply this patch. I can do it but I'm on vacation this week with limited access to the internet.

@esabol
Copy link
Contributor

esabol commented Aug 2, 2023

@vitcpp wrote:

I propose to enable and fix PG16 tests in another PR and then apply this patch. I can do it but I'm on vacation this week with limited access to the internet.

This sounds to me like a very good plan.

Thank you, @df7cb, for detailing the issues with the test suite and such.

@dura0ok
Copy link

dura0ok commented Aug 2, 2023

@df7cb I cherry-picked all your commits to my branch, made a small change and the tests passed.
You can see in #41
@vitcpp @esabol what do you think?

@vitcpp
Copy link
Contributor

vitcpp commented Aug 14, 2023

@df7cb We fixed some warning messages that resulted in test failures. Could you please be so kind to re-sync your branch? Thank you in advance!

df7cb added 3 commits August 16, 2023 17:22
* Replace Abs with fabs
* Remove DatumGetPointer where PG16 got stricter typechecking
* Annotate expected/moc100 output files with PG major and architecture
  bits they are meant for
* Add expected output files for moc100 on PG16
@df7cb
Copy link
Contributor Author

df7cb commented Aug 16, 2023

Branch rebased.

Copy link
Contributor

@esabol esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Let's merge. Thank you , @df7cb !

@vitcpp vitcpp merged commit 1709fd5 into postgrespro:master Aug 16, 2023
@df7cb df7cb deleted the pg16 branch October 5, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants